feat: forward client request headers to upstream providers in bridge routes#195
feat: forward client request headers to upstream providers in bridge routes#195ssncferreira wants to merge 2 commits intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ssncferreira
force-pushed
the
ssncf/feat-forward-client-headers
branch
5 times, most recently
from
4544e46 to
8c0c002
Compare
ssncferreira
force-pushed
the
ssncf/feat-forward-client-headers
branch
2 times, most recently
from
78cdc43 to
2a43c3e
Compare
| return nil, fmt.Errorf("unmarshal request body: %w", err) | ||
| } | ||
|
|
||
| cfg := p.cfg |
There was a problem hiding this comment.
nit: remove cfg := p.cfg and use p.cfg line below
| // below takes priority on any conflict. | ||
| for k, vals := range intercept.SanitizeClientHeaders(i.clientHeaders) { | ||
| for _, v := range vals { | ||
| opts = append(opts, option.WithHeader(k, v)) |
There was a problem hiding this comment.
WithHeader uses Header.Set which overrides header value instead of adding it.
I think WithHeaderAdd would help here. Test case needed.
There was a problem hiding this comment.
For some headers, it makes sense to use Add, but for others not so much. For instance, the SDK sets X-Stainless-* headers, and the client sends its own. Using Add, the upstream would receive both values. Using Set, the upstream sees only the client's value, which also does not seem correct since AI Bridge is the one making the request.
This is a good point, and makes me wonder if this is the right direction 🤔 I added this as a topic to discuss at the weekly
|
|
||
| func (i *interceptionBase) newCompletionsService() openai.ChatCompletionService { | ||
| opts := []option.RequestOption{option.WithAPIKey(i.cfg.Key), option.WithBaseURL(i.cfg.BaseURL)} | ||
| var opts []option.RequestOption |
There was a problem hiding this comment.
option order matters? I'd assume it should not so old way of initializing opts could stay?
| // below takes priority on any conflict. | ||
| for k, vals := range intercept.SanitizeClientHeaders(i.clientHeaders) { | ||
| for _, v := range vals { | ||
| opts = append(opts, option.WithHeader(k, v)) |
There was a problem hiding this comment.
WithHeader -> WithHeaderAdd
ssncferreira
force-pushed
the
ssncf/feat-forward-client-headers
branch
from
9b666ae to
2b13d58
Compare
|
@ssncferreira shall we close this, following our discussion? |
|
Closing in favor of #214 |
3 participants
Description
AI Bridge drops client request headers when making upstream API calls through bridge routes. The interceptors construct new HTTP requests via provider SDKs, which set their own headers and discard the originals.
Rather than managing specific headers per provider (as done with Copilot's ExtraHeaders), this PR forwards all client headers to upstream providers, stripping only those that are unsafe or managed by the SDK.
Changes
intercept/client_headers.gowithSanitizeClientHeadersthat clones client headers and strips hop-by-hop and transport-managed headers before forwarding.Follow-up
The following changes will be addressed in follow-up PRs to reduce scope:
X-Forwarded-For,X-Forwarded-Host,X-Forwarded-Proto), while bridge routes do not. Since AI Bridge behaves as a proxy, this handling should be consistent across all requests.ExtraHeadersmechanism introduced for Copilot redundant, as all client headers are now forwarded. It can be safely removed.ExtraHeadersmechanism introduced for Anthropic in #205 redundant, as all client headers are now forwarded. It can be safely removed.Closes: #192